Add allOf support for model definitions (#98) #312
Conversation
Collapses the child elements into one, without class heirarchy, mixins, etc.
bowenwr
force-pushed
the
benchling-allof-support
branch
from
c7bc39e to
a7b6d47
Compare
Codecov Report
@@ Coverage Diff @@
## main #312 +/- ##
===========================================
- Coverage 100.00% 99.65% -0.35%
===========================================
Files 47 47
Lines 1385 1439 +54
===========================================
+ Hits 1385 1434 +49
- Misses 0 5 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for contributing this upstream! I left a smattering of comments around about style/performance, but I think there might be an overall more elegant solution. Is there a reason we can't just look for the references in schemas in build_model_property? Then return a PropertyError if it's not found? I think the loop in build_schemas would then catch circular dependencies (which would just infinite loop with this implementation I think) and eventually resolve everything.
That should also clear up a bunch of logic around the recursion, optional vs required stuff, forward references, etc. and prevent folks from accidentally forgetting to resolve_references.
I could totally be missing some issue with that approach in here, maybe I'm making it sound simpler than it actually is 😅 but let me know.
| if not isinstance(data, oai.Reference) and data.allOf: | ||
| for sub_prop in data.allOf: | ||
| if isinstance(sub_prop, oai.Reference): | ||
| references += [sub_prop] |
There was a problem hiding this comment.
| references += [sub_prop] | |
| references.append(sub_prop) |
I'm not completely sure on the internals of how append vs + works, but I think this is probably faster since += reassigns the array.
| models = list(schemas.models.values()) | ||
| for model in models: |
There was a problem hiding this comment.
| models = list(schemas.models.values()) | |
| for model in models: | |
| for model in schemas.models.values(): |
I don't see a reason to construct a new list just for iterating.
| @@ -486,8 +494,10 @@ def _property_from_data( | |||
| ) | |||
| elif data.type == "array": | |||
| return build_list_property(data=data, name=name, required=required, schemas=schemas, parent_name=parent_name) | |||
| elif data.type == "object": | |||
| elif data.type == "object" or data.allOf: | |||
| return build_model_property(data=data, name=name, schemas=schemas, required=required, parent_name=parent_name) | |||
There was a problem hiding this comment.
I think you have to call resolve_references here in order to not miss any references in allOf from non-component schemas.
| while self.references: | ||
| reference = self.references.pop() | ||
| source_name = Reference.from_ref(reference.ref).class_name | ||
| referenced_prop = components[source_name] |
There was a problem hiding this comment.
Could get an unhandled KeyError here, need to check that the component actually exists and return a PropertyError so it's handled gracefully.
| reference = self.references.pop() | ||
| source_name = Reference.from_ref(reference.ref).class_name | ||
| referenced_prop = components[source_name] | ||
| assert isinstance(referenced_prop, oai.Schema) |
There was a problem hiding this comment.
Generally better to cast than to assert if just providing type info. Although there actually is no guarantee that this is true, is there? So this would really have to be a runtime check and return PropertyError
| if isinstance(referenced_prop.required, Iterable): | ||
| for sub_prop_name in referenced_prop.required: | ||
| required_set.add(sub_prop_name) |
There was a problem hiding this comment.
| if isinstance(referenced_prop.required, Iterable): | |
| for sub_prop_name in referenced_prop.required: | |
| required_set.add(sub_prop_name) | |
| for sub_prop_name in referenced_prop.required or []: | |
| required_set.add(sub_prop_name) |
Just sticking with the same form we usually use for optional iterables. Although maybe making the schema be a Set with a default would be cleaner all around.
| for sub_prop_name in referenced_prop.required: | ||
| required_set.add(sub_prop_name) | ||
|
|
||
| for key, (value, source_name) in (props or {}).items(): |
There was a problem hiding this comment.
| for key, (value, source_name) in (props or {}).items(): | |
| for key, (value, source_name) in props.items(): |
Doesn't look like props can me None to me
| if required: | ||
| self.required_properties.append(prop) | ||
| # Remove the optional version | ||
| new_optional_props = [op for op in self.optional_properties if op.name != prop.name] | ||
| self.optional_properties.clear() | ||
| self.optional_properties.extend(new_optional_props) | ||
| elif not any(ep for ep in (self.optional_properties + self.required_properties) if ep.name == prop.name): | ||
| self.optional_properties.append(prop) |
There was a problem hiding this comment.
This all makes me feel like we should make the properties lists sets instead, and make Property hashable on name? Or make them dicts keyed by name? Would save us a lot of cycles on lists when doing things like this.
Mentioned in the PR as well, but I attempted this in #321 to show what I was talking about, so you may want to check that before responding to any of my other comments in this PR. |
|
Closing in favor of #321 |
Fixes #98. Collapses the child elements into one, without class hierarchy, mixins, etc.